Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

change identity comparisons to use isequal #16764

Merged
merged 2 commits into from
Jun 8, 2016
Merged

change identity comparisons to use isequal #16764

merged 2 commits into from
Jun 8, 2016

Conversation

vtjnash
Copy link
Member

@vtjnash vtjnash commented Jun 4, 2016

this makes it now possible to make == with non-comparable items an error (ref #15983), since these notions are no longer coupled, but it does not make that change now

it does change in to use isequal (#9381), but that's a small part of the change, so I might move that to another branch while that issue is still under discussion edit: now extracted from this PR to jn/notequal...jn/in-isequal

@JeffBezanson
Copy link
Member

Yes, the behavior change to in should definitely be separated; the rest is uncontroversial cleanup to use === and isequal appropriately.

end
findlast(A, v) = findprev(A, v, length(A))

# returns the index of the previous element for which the function returns true, or zero if it never does
function findprev(testf::Function, A, start::Integer)
for i = start:-1:1
testf(A[i]) && return i
if testf(A[i])
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why make this change here and not above? (Anyway, that's clearly unrelated to this PR...)

@StefanKarpinski
Copy link
Member

the rest is uncontroversial cleanup to use === and isequal appropriately.

Agreed: this should be in its own PR and merged immediately.

vtjnash added 2 commits June 7, 2016 13:18
it would now be possible to make == with non-comparable items an error
since these notions are no longer coupled

fix #9381
ref #15983
the external abi is to call var,
the internal abi doesn' need to branch to alternative functions based on whether mean is given as zero
that simply made the dispatch less straightforward to understand
even though doing an exact comparison to 0 isn't generally reliable

ref #6273, which initially introduced these functions as the public API, before changing them to be the internal implementation
@vtjnash
Copy link
Member Author

vtjnash commented Jun 8, 2016

merge immediately

I'll merge in the morning, if there's no further comments

mode = have_file ? "r+" : "w+"
end
workermode = mode == "w+" ? "r+" : mode # workers don't truncate!

# Ensure the file will be readable
mode in ("r", "r+", "w+", "a+") || throw(ArgumentError("mode must be readable, but $mode is not"))
init==false || mode in ("r+", "w+", "a+") || throw(ArgumentError("cannot initialize unwritable array (mode = $mode)"))
if init !== false
typeassert(init, Function)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This typeassert seems to add a restriction that was not here before.

Copy link
Member Author

@vtjnash vtjnash Jun 8, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good call. I added this so that init isn't silently ignored later (https://github.com/JuliaLang/julia/pull/16764/files/e6e1312a051d9b5511bf5b4f9ea0ae97528bb5ed#diff-6ba74fe8f18fa73bec8dfbfd1801a05cL209), but that test is wrong too

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what are you trying to link to?

@vtjnash vtjnash merged commit 7c36704 into master Jun 8, 2016
@vtjnash vtjnash deleted the jn/notequal branch June 8, 2016 20:38
@malmaud
Copy link
Contributor

malmaud commented Jun 9, 2016

This broke JuliaStats/StatsBase.jl#169, which depends on varzm.

@tkelman
Copy link
Contributor

tkelman commented Jun 9, 2016

should either revert e6e1312 or add a deprecation. DataArrays uses it too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants